Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Sampler.ShouldSample parent parameter to be Context #881

Merged
merged 10 commits into from
Oct 20, 2020

Conversation

Oberon00
Copy link
Member

@Oberon00 Oberon00 commented Aug 26, 2020

Fixes #1024

Changes

  • Change Sampler.ShouldSample to receive a full parent Context

CC @alolita (you are responsible for the sampling issues currently)

specification/trace/sdk.md Outdated Show resolved Hide resolved
specification/trace/sdk.md Outdated Show resolved Hide resolved
@Oberon00 Oberon00 added area:sampling Related to trace sampling area:sdk Related to the SDK release:required-for-ga Must be resolved before GA release, or nice to have before GA spec:trace Related to the specification/trace directory labels Aug 26, 2020
@Oberon00

This comment has been minimized.

@Oberon00 Oberon00 changed the title Update Sampler.ShouldSample parent parameter to be Context, clean up. Update Sampler.ShouldSample parent parameter to be Context. Aug 26, 2020
@carlosalberto

This comment has been minimized.

@bogdandrutu

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions github-actions bot added the Stale label Sep 9, 2020
@Oberon00 Oberon00 marked this pull request as draft September 9, 2020 08:30
@carlosalberto
Copy link
Contributor

@bogdandrutu out of curiosity, of specifying Context as the single source of parenthood goes in, this would become required for GA?

@github-actions github-actions bot removed the Stale label Sep 17, 2020
@carlosalberto
Copy link
Contributor

@bogdandrutu @Oberon00 You guys think is is needed after the related PR was merged?

cc @alolita

@Oberon00 Oberon00 marked this pull request as ready for review September 24, 2020 08:12
@Oberon00
Copy link
Member Author

@carlosalberto I think "needed" is the wrong word, just the SpanContext should work as well as before it is not needed more than before but I think it now makes even more sense. Note that among other things, this gives access to the full parent span in many cases, instead of only the SpanContext.

@Oberon00 Oberon00 removed the release:required-for-ga Must be resolved before GA release, or nice to have before GA label Sep 28, 2020
@Oberon00 Oberon00 changed the title Update Sampler.ShouldSample parent parameter to be Context. Update Sampler.ShouldSample parent parameter to be Context Sep 28, 2020
@arminru arminru requested a review from a team October 6, 2020 12:27
@Oberon00 Oberon00 added the release:allowed-for-ga Editorial changes that can still be added before GA since they don't require action by SIGs label Oct 6, 2020
@Oberon00
Copy link
Member Author

I think this is really a straightforward, simple to implement (maybe even simplifying) and beneficial change since #875 and also #994 was merged some time ago. @open-telemetry/specs-approvers please review!

@arminru
Copy link
Member

arminru commented Oct 13, 2020

Please add an entry in the spec compliance matrix (preferably marking it as "[SDK]" or something in case we'll split the table later on).

@Oberon00
Copy link
Member Author

It feels like the changelog and compliance matrix should somehow be merged...

I'll add the line in the table.

@Oberon00
Copy link
Member Author

Done in f081f86.
I did not add the [SDK] prefix since it's in a Sampling section which is all-SDK anyway.

@carlosalberto
Copy link
Contributor

@open-telemetry/specs-approvers Please review. Although not something required-for-GA, it's still allowed to go in and looks straight enough ;)

@Oberon00
Copy link
Member Author

This PR has been semantically unchanged since 25 days ago and it has two approvers from different companies, so can we just merge it? @open-telemetry/technical-committee

Otherwise I will bring it up in the SIG spec meeting tomorrow, unless it can get more reviews until then 😃 @open-telemetry/specs-approvers @open-telemetry/specs-trace-approvers

@carlosalberto
Copy link
Contributor

@Oberon00 Lets mention it tomorrow, please (although I'm strongly inclined to merge it already, as it's a natural progression of allowing Context to be the only parenthood source).

Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discard all my previous comments, I think this is a good change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:sampling Related to trace sampling area:sdk Related to the SDK release:allowed-for-ga Editorial changes that can still be added before GA since they don't require action by SIGs spec:trace Related to the specification/trace directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sampler.ShouldSample parent parameter should be Context
5 participants